Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Localized artifacts now contain Authenticode signed assemblies #6172

Open
wants to merge 2 commits into
base: dev
Choose a base branch
from

Conversation

donnie-msft
Copy link
Contributor

Bug

Fixes:

Description

Documentation update for #6170

PR Checklist

  • Meaningful title, helpful description and a linked NuGet/Home issue
  • Added tests
  • Link to an issue or pull request to update docs if this PR changes settings, environment variables, new feature, etc.

jeffkl
jeffkl previously approved these changes Nov 27, 2024
@@ -24,9 +24,11 @@ Unfortunately, NuGet.Client does not use OneLocBuild, and any resource comments
Although the steps below are for the dev branch, the process is similar for release branches.

1. A PR merges into the dev branch.
1. A NuGet.Client build generates a `localizationArtifacts` artifact. This artifact contains newly built (but unsigned) assemblies and .lcg files. The .lcg files contain information extracted from .resx files and are the primary input for later localization.
1. A NuGet.Client build generates a `localizationArtifacts` artifact. This artifact contains newly built Authenticode-signed assemblies [(see PR#6170)][12]) containing English resources and .lcg files.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
1. A NuGet.Client build generates a `localizationArtifacts` artifact. This artifact contains newly built Authenticode-signed assemblies [(see PR#6170)][12]) containing English resources and .lcg files.
1. A NuGet.Client build generates a `localizationArtifacts` artifact. This artifact contains newly built assemblies containing English resources and .lcg files.

If someone needs to investigate the loc process in a year or more from now, I'm not sure why the PR would be interesting to call out specifically. In fact, the fact that it's signed or not also doesn't seem important to me.

The loc team's tools don't have a requirement about signed or unsigned assemblies.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The loc team's tools don't have a requirement about signed or unsigned assemblies.

This documentation is for the NuGet Client team, not the loc team.

In fact, the fact that it's signed or not also doesn't seem important to me.

The original document indicated that these were unsigned assemblies. This is an update reflecting the new state of these assemblies.
This change is part of the SFI work to ensure we sign assemblies. If localized assemblies are not signed, we are no longer meeting SFI requirements.

docs/localizability.md Show resolved Hide resolved
[11]: https://devdiv.visualstudio.com/DevDiv/_wiki/wikis/DevDiv.wiki/20709/MicroBuild-Localization
[12]: https://github.com/NuGet/NuGet.Client/pull/6170
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar to another comment, it's not clear to me why someone reading these docs would be more interested in this pull request than any other pull request that modified build/loc.proj or the build yaml.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This pull request contains an explanation of why it's important to sign the localized assemblies, as well as how it's being done. It's not accidental that they're signed, it is intentional, so the documentation is referencing that PR to point to the purpose of that behavior.

@jeffkl jeffkl self-requested a review December 3, 2024 23:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants